-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-4179): allow secureContext in KMS TLS options #4578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
const { AlpineTestConfiguration } = require('../../tools/runner/config'); | ||
|
||
const getKmsProviders = (localKey, kmipEndpoint, azureEndpoint, gcpEndpoint) => { | ||
import { BSON, EJSON } from 'bson'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note there is no related change to this feature in this test. I needed to convert it to TS because it started breaking everything at module load with:
[2025/07/08 22:03:11.087] Exception during run: ReferenceError: require is not defined in ES module scope, you can use import instead
[2025/07/08 22:03:11.087] at file:///data/mci/f715752cc296c929fd848c3df13d321a/src/test/integration/client-side-encryption/client_side_encryption.prose.test.js:2:14
[2025/07/08 22:03:11.087] at ModuleJob.run (node:internal/modules/esm/module_job:329:25)
[2025/07/08 22:03:11.087] at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:644:26)
[2025/07/08 22:03:11.087] at async formattedImport (/data/mci/f715752cc296c929fd848c3df13d321a/src/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
[2025/07/08 22:03:11.087] at async Object.exports.requireOrImport (/data/mci/f715752cc296c929fd848c3df13d321a/src/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
[2025/07/08 22:03:11.087] at async Object.exports.loadFilesAsync (/data/mci/f715752cc296c929fd848c3df13d321a/src/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
[2025/07/08 22:03:11.087] at async singleRun (/data/mci/f715752cc296c929fd848c3df13d321a/src/node_modules/mocha/lib/cli/run-helpers.js:162:3)
[2025/07/08 22:03:11.087] at async Object.exports.handler (/data/mci/f715752cc296c929fd848c3df13d321a/src/node_modules/mocha/lib/cli/run.js:375:5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the conversion, but what caused that? It would be good to at least understand where this is coming from and why now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I could confirm is that my changes in driver.test.ts somehow caused this to happen, even though it doesn't reference the prose test at all. It happens on all Node versions. I didn't want to spend too much time getting down to the exact reason and just figured converting it now would be faster.
15cf4a6
to
75cb045
Compare
if (tlsOptions.tlsCertificateKeyFilePassword) { | ||
options.passphrase = tlsOptions.tlsCertificateKeyFilePassword; | ||
// If a secureContext is provided, it takes precedence over the other options. | ||
if (tlsOptions.secureContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be making an analogous change in the driver in, e.g., V7? we don't currently do any overrides for it in the main client logic (see _connect()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would make sense to me here for sure. I'd also consider the possibility of taking either an existing SecureContext object or the options object needed to create one via tls.createSecureContext()
– keeping the overall MongoClient options object (including AutoEncryptionOptions) mostly (de)serializable through standard formats like JSON is generally a helpful best practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@durran Were you able to verify the behavior for what happens when we pass both secureContext and the standalone options to Node in these two cases: (1) secureContext has valid auth, standalone options have invalid auth and (2) secureContext has invalid auth, standalone options have valid auth? If in both cases the secureContext is used and the other options are ignored, we should be able to make the same change to the driver option handling right now for consistency. Otherwise please file the V7 ticket to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll file a ticket for V7. In my testing when providing both a secure context and other TLS options, the connection just hangs and then times out (https://evergreen.mongodb.com/test_log/mongo_node_driver_next_rhel80_large_node_latest_test_tls_support_latest_patch_bff57ed87b236e7840a700989f3287f1be856fa9_68701e519bc80f00076e5617_25_07_10_20_11_06/0?test_name=b8c798e2ba619290e1fe4c562783bdc7#L0) so it doesn't seem like there's any real validation.
@@ -111,7 +111,7 @@ declare module 'mongodb-client-encryption' { | |||
*/ | |||
export type ClientEncryptionTlsOptions = Pick< | |||
MongoClientOptions, | |||
'tlsCAFile' | 'tlsCertificateKeyFile' | 'tlsCertificateKeyFilePassword' | |||
'tlsCAFile' | 'tlsCertificateKeyFile' | 'tlsCertificateKeyFilePassword' | 'secureContext' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a blocker for us, but is there a reason not to accept other options that the driver passes through for regular connections (i.e. what's listed in LEGAL_TLS_SOCKET_OPTIONS
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope of the ticket was for secureContext
. There's no reason not to allow the others but I'd open a new ticket for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax does that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although I'll say that the intention when the ticket was first created was absolutely not just scoped to secureContext
only. And I do feel like consistency between the KMS TLS handling and the main driver TLS handling should be an implicit goal in some form here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax fully agree - in retrospect we (the node team) should have been more diligent about stating our implicit assumptions in the ticket requirements; it's also a good reminder for us to always follow up with our stakeholders after refinement, whenever possible, to make sure we are all aligned; sorry about this, we'll try to do better next time :)
@durran do you want to go ahead and file that follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (tlsOptions.tlsCertificateKeyFilePassword) { | ||
options.passphrase = tlsOptions.tlsCertificateKeyFilePassword; | ||
// If a secureContext is provided, it takes precedence over the other options. | ||
if (tlsOptions.secureContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@durran Were you able to verify the behavior for what happens when we pass both secureContext and the standalone options to Node in these two cases: (1) secureContext has valid auth, standalone options have invalid auth and (2) secureContext has invalid auth, standalone options have valid auth? If in both cases the secureContext is used and the other options are ignored, we should be able to make the same change to the driver option handling right now for consistency. Otherwise please file the V7 ticket to do so.
Description
Allows users to pass a
secureContext
option to the TLS options in client encryption and auto encryption.What is changing?
secureContext
option to thetlsOptions:<provider>
option inautoEncryption
options on theMongoClient
or the options forClientEncryption
.secureContext
option takes precedence over the driver tls* options, that the tls* options aren't attempted to be read from the file, and that it works end-to-end.Is there new documentation needed for these changes?
Yes, update the MongoDB manual to show the precedence of the options.
What is the motivation for this change?
NODE-4179
Release Highlight
Allow a
secureContext
for Auto Encryption and Client Encryption TLS optionsThis can be provided in the
tlsOptions
option both both objects.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript